[Guardrails] Banlist API #36
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds ban-list support: database migration, SQLModel data model, schemas, CRUD layer, FastAPI routes wired into main router, unit and integration tests, test seed data/fixtures, and a dependency bump for guardrails-ai[hub] to >=0.8.0. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as "FastAPI (/guardrails/ban-lists)"
participant CRUD as "banlist_crud"
participant DB as "Database (banlist table)"
Client->>API: POST /guardrails/ban-lists (payload, tenant headers)
API->>CRUD: create(data, organization_id, project_id)
CRUD->>DB: INSERT banlist row
DB-->>CRUD: return created row
CRUD-->>API: return BanList object
API-->>Client: 201 APIResponse(BanList)
Client->>API: GET /guardrails/ban-lists?domain=...
API->>CRUD: list(organization_id, project_id, domain)
CRUD->>DB: SELECT ... WHERE org_id/ proj_id OR is_public
DB-->>CRUD: return rows
CRUD-->>API: list[BanList]
API-->>Client: 200 APIResponse(list)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@backend/app/alembic/versions/004_added_banlist_config.py`:
- Around line 21-41: The migration creates the ban_list table without a
uniqueness constraint, so the IntegrityError handling in BanListCrud.create and
BanListCrud.update is dead code; either add a unique constraint to enforce
unique (name, organization_id, project_id) or remove that exception handling. To
fix by enforcing uniqueness, update the upgrade() in the
004_added_banlist_config.py migration to add a UniqueConstraint on ('name',
'organization_id', 'project_id') (e.g., include
sa.UniqueConstraint('name','organization_id','project_id') in op.create_table or
call op.create_unique_constraint after table creation) and add the corresponding
drop (op.drop_constraint) in downgrade(); reference the ban_list table and
BanListCrud.create / BanListCrud.update so the CRUD IntegrityError handling
becomes meaningful.
In `@backend/app/api/routes/banlist_configs.py`:
- Around line 64-78: The update_banlist and delete_banlist routes allow mutating
public ban lists because banlist_crud.get() skips ownership for obj.is_public
and banlist_crud.update/delete do not verify owner; add an explicit ownership
check in both route handlers after fetching obj (in update_banlist and
delete_banlist) that compares the object's owner (obj.organization_id or
obj.owner_id) against the caller's organization (use the AuthDep value currently
ignored in the signature or the organization_id parameter) and raise an
authorization error (HTTP 403) if they differ; ensure the check runs for
mutating operations on public lists so only the owning organization can update
or delete, then proceed to call banlist_crud.update or banlist_crud.delete when
the ownership check passes.
- Around line 24-32: create_banlist currently calls banlist_crud.create without
verifying the caller's tenant membership; before invoking banlist_crud.create,
extract the authenticated user's identity from the AuthDep (or accept/extend
AuthDep to return a user object), then verify that user belongs to the provided
organization_id and project_id (either by calling the existing check_owner()
helper in the CRUD layer or by querying the membership via the SessionDep), and
only proceed to call banlist_crud.create if that validation succeeds; if
validation fails, return an appropriate authorization error instead of creating
the banlist.
In `@backend/app/crud/banlist.py`:
- Around line 14-38: The create method for BanList currently only catches
IntegrityError, so it must mirror the update/delete behavior: wrap
session.commit() in a try block that catches IntegrityError (raising the 400
HTTPException) and also catches a generic Exception to session.rollback() and
re-raise as an HTTPException(500) (or propagate after rollback) to ensure the DB
session is not left broken; update the create function (BanList.create, where
session.commit() is called after session.add(obj) and before
session.refresh(obj)) to perform the same rollback-on-any-exception logic used
by update and delete.
In `@backend/app/models/config/banlist.py`:
- Around line 26-29: The BanList model's description field is declared
Optional[str] but marked nullable=False, causing a mismatch; change the type
annotation on the description field from Optional[str] to str (i.e., make it
required) so it aligns with sa_column nullable=False and the ORM won't try to
insert NULL, or alternatively set sa_column_kwargs nullable=True if you want to
allow nulls—update the declaration for the description Field in the BanList
model accordingly (adjust the type annotation and/or the sa_column_kwargs
nullable flag).
- Around line 53-58: The Field declaration for domain is inconsistent: it's
typed as str but given default=None while sa_column_kwargs sets nullable=False;
fix by making domain required—remove default=None from the Field for the domain
attribute in banlist.py (keep type str and nullable=False) so the
Pydantic/SQLAlchemy model enforces a value on creation; update any code that
instantiates the model to provide domain where missing.
In `@backend/app/schemas/banlist.py`:
- Around line 8-9: BanListBase currently sets model_config =
ConfigDict(extra="allow"), which lets BanListCreate accept arbitrary request
fields that then flow into the CRUD create path (see
BanList(**data.model_dump(), ...) in the create method); change BanListBase to
forbid extra fields (use ConfigDict(extra="forbid") or remove the model_config
so default behavior applies) so unexpected fields are rejected at validation
time and cannot reach the create logic.
🧹 Nitpick comments (13)
backend/pyproject.toml (1)
25-25: Consider adding an upper-bound pin forguardrails-ai.Most dependencies in this file use an upper bound (e.g.,
<X.0.0), butguardrails-ai[hub]>=0.8.0is open-ended. A major version bump could introduce breaking changes. Consider constraining it, e.g.,guardrails-ai[hub]>=0.8.0,<1.0.0.backend/app/tests/test_banlist_configs.py (2)
113-125: Assert thatcrud.deletewas actually called.
test_delete_successonly checksresult.successbut never verifies thatcrud.deletewas invoked. Without this, the test would pass even if the delete call were accidentally removed from the route handler.Proposed fix
result = delete_banlist( id=TEST_ID, organization_id=TEST_ORGANIZATION_ID, project_id=TEST_PROJECT_ID, session=mock_session, _=None, ) assert result.success is True + crud.delete.assert_called_once_with(mock_session, sample_banlist)
53-65: Consider assertingcrud.createwas called with the expected arguments.Same pattern applies to the other tests — verifying that the CRUD method was called with the correct arguments strengthens confidence that the route is wiring parameters correctly, not just returning a canned mock.
backend/app/schemas/banlist.py (1)
1-3: Remove unused imports:datetimeandList.Neither
datetimenorListare used in this file. Ruff also flagsList(UP035).Proposed fix
from uuid import UUID -from datetime import datetime -from typing import List, Optional +from typing import Optionalbackend/app/tests/test_banlists_integration.py (2)
65-69: Method namelistshadows the Python built-in.Consider renaming to
list_banlistsorlist_allto avoid shadowingbuiltins.list. It won't break anything here, but it can cause subtle issues if someone later useslist(...)inside this class.
81-98: Consider adding a test for duplicate creation (IntegrityError path).The CRUD layer handles
IntegrityErrorand returns a 400. This error path isn't exercised by any integration test. A test creating the same ban list twice would cover it.backend/app/alembic/versions/004_added_banlist_config.py (2)
31-32:created_atandupdated_atlackserver_default.If rows are ever inserted outside the ORM (manual SQL, other services, data migrations), these columns will fail the
NOT NULLconstraint. Consider addingserver_default=sa.func.now()for resilience.
42-43: Downgrade should drop indexes explicitly before dropping the table.While PostgreSQL automatically drops indexes when the table is dropped, being explicit in the downgrade makes the migration symmetric and portable across databases.
backend/app/models/config/banlist.py (1)
1-3: Remove unused importList.
Listfromtypingis imported but not used; the code uses the built-inlist[str]syntax throughout. Ruff also flags this (UP035).Proposed fix
from datetime import datetime -from typing import List, Optional +from typing import Optional from uuid import UUID, uuid4backend/app/crud/banlist.py (3)
57-75: No pagination on thelistquery — may return unbounded results.The
listmethod returns all matching records (own + all public from every org). As the number of public ban lists grows, this could become a performance issue. Consider addinglimit/offset(or cursor-based) pagination.
2-2: Use built-inlistinstead oftyping.List.
typing.Listis deprecated since Python 3.9. The built-inlistis already used elsewhere in this codebase (e.g.,list[str]in the model). This aligns with the Ruff UP035 hint.Proposed fix
-from typing import List, Optional +from typing import OptionalAnd on line 63:
- ) -> List[BanList]: + ) -> list[BanList]:
118-118: Add trailing newline at end of file.Per Ruff W292 and POSIX convention.
backend/app/api/routes/banlist_configs.py (1)
34-46: List endpoint looks correct, but inherits the pagination concern from the CRUD layer.As noted in the CRUD review, the
listmethod returns unbounded results. Consider addinglimit/offsetquery params here to support pagination.
| def create_banlist( | ||
| payload: BanListCreate, | ||
| session: SessionDep, | ||
| organization_id: int, | ||
| project_id: int, | ||
| _: AuthDep, | ||
| ): | ||
| response_model = banlist_crud.create(session, payload, organization_id, project_id) | ||
| return APIResponse.success_response(data=response_model) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*.py" | xargs grep -l "AuthDep" | head -20Repository: ProjectTech4DevAI/kaapi-guardrails
Length of output: 232
🏁 Script executed:
cat -n ./backend/app/api/deps.pyRepository: ProjectTech4DevAI/kaapi-guardrails
Length of output: 1438
🏁 Script executed:
cat -n ./backend/app/api/routes/banlist_configs.pyRepository: ProjectTech4DevAI/kaapi-guardrails
Length of output: 3047
🏁 Script executed:
find . -type f -name "*.py" -path "*/crud/*" | xargs grep -l "banlist" | head -5Repository: ProjectTech4DevAI/kaapi-guardrails
Length of output: 107
🏁 Script executed:
cat -n ./backend/app/crud/banlist.pyRepository: ProjectTech4DevAI/kaapi-guardrails
Length of output: 3936
🏁 Script executed:
find . -type f -name "*.py" | xargs grep -l "middleware\|tenant\|organization_id" | grep -E "middleware|main|app\.py" | head -10Repository: ProjectTech4DevAI/kaapi-guardrails
Length of output: 99
🏁 Script executed:
cat -n ./backend/app/main.pyRepository: ProjectTech4DevAI/kaapi-guardrails
Length of output: 1365
🏁 Script executed:
find . -type f -name "*.py" -path "*/middleware*" -o -name "middleware.py" | head -5Repository: ProjectTech4DevAI/kaapi-guardrails
Length of output: 110
🏁 Script executed:
cat -n ./backend/app/core/middleware.pyRepository: ProjectTech4DevAI/kaapi-guardrails
Length of output: 883
Apply tenant isolation validation in create_banlist() to prevent privilege escalation.
The create() CRUD method accepts organization_id and project_id as query parameters without validating that the authenticated user has access to them. Any authenticated user can create banlists for arbitrary organizations by passing different query parameter values. While AuthDep validates the bearer token, it provides no user context or tenant affiliation. check_owner() exists in the CRUD layer but is only invoked for reads/updates/deletes on public resources—not for creation.
Add validation before calling banlist_crud.create() to ensure the authenticated user belongs to the specified organization and project, or extract user context from the bearer token to enforce this check.
🤖 Prompt for AI Agents
In `@backend/app/api/routes/banlist_configs.py` around lines 24 - 32,
create_banlist currently calls banlist_crud.create without verifying the
caller's tenant membership; before invoking banlist_crud.create, extract the
authenticated user's identity from the AuthDep (or accept/extend AuthDep to
return a user object), then verify that user belongs to the provided
organization_id and project_id (either by calling the existing check_owner()
helper in the CRUD layer or by querying the membership via the SessionDep), and
only proceed to call banlist_crud.create if that validation succeeds; if
validation fails, return an appropriate authorization error instead of creating
the banlist.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@AkhileshNegi a bit confused. When kaapi-guardrails makes an API call to kaapi (/login/access-token) with the auth-token, we assumed kaapi will do the authentication and authorization work. We don't store user information to be able to check whether this user belongs to this org or project.
| @router.patch( | ||
| "/{id}", | ||
| response_model=APIResponse[BanListResponse] | ||
| ) | ||
| def update_banlist( | ||
| id: UUID, | ||
| organization_id: int, | ||
| project_id: int, | ||
| payload: BanListUpdate, | ||
| session: SessionDep, | ||
| _: AuthDep, | ||
| ): | ||
| obj = banlist_crud.get(session, id, organization_id, project_id) | ||
| response_model = banlist_crud.update(session, obj=obj, data=payload) | ||
| return APIResponse.success_response(data=response_model) |
There was a problem hiding this comment.
Critical: Any authenticated user can update (and delete) public ban lists they don't own.
The update_banlist route calls banlist_crud.get() to fetch the object, but get() skips the ownership check when obj.is_public is True. The update() CRUD method itself performs no ownership verification either. This means any authenticated user can modify any public ban list created by another organization.
The same issue applies to delete_banlist (lines 80–93).
Proposed fix — enforce ownership on mutating operations
Either add an explicit ownership check in the route before mutating:
def update_banlist(
id: UUID,
organization_id: int,
project_id: int,
payload: BanListUpdate,
session: SessionDep,
_: AuthDep,
):
obj = banlist_crud.get(session, id, organization_id, project_id)
+ banlist_crud.check_owner(obj, organization_id, project_id)
response_model = banlist_crud.update(session, obj=obj, data=payload)
return APIResponse.success_response(data=response_model)And similarly for delete_banlist:
def delete_banlist(
id: UUID,
organization_id: int,
project_id: int,
session: SessionDep,
_: AuthDep,
):
obj = banlist_crud.get(session, id, organization_id, project_id)
+ banlist_crud.check_owner(obj, organization_id, project_id)
banlist_crud.delete(session, obj)
return APIResponse.success_response(data={"message": "Banlist deleted successfully"})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @router.patch( | |
| "/{id}", | |
| response_model=APIResponse[BanListResponse] | |
| ) | |
| def update_banlist( | |
| id: UUID, | |
| organization_id: int, | |
| project_id: int, | |
| payload: BanListUpdate, | |
| session: SessionDep, | |
| _: AuthDep, | |
| ): | |
| obj = banlist_crud.get(session, id, organization_id, project_id) | |
| response_model = banlist_crud.update(session, obj=obj, data=payload) | |
| return APIResponse.success_response(data=response_model) | |
| `@router.patch`( | |
| "/{id}", | |
| response_model=APIResponse[BanListResponse] | |
| ) | |
| def update_banlist( | |
| id: UUID, | |
| organization_id: int, | |
| project_id: int, | |
| payload: BanListUpdate, | |
| session: SessionDep, | |
| _: AuthDep, | |
| ): | |
| obj = banlist_crud.get(session, id, organization_id, project_id) | |
| banlist_crud.check_owner(obj, organization_id, project_id) | |
| response_model = banlist_crud.update(session, obj=obj, data=payload) | |
| return APIResponse.success_response(data=response_model) |
🤖 Prompt for AI Agents
In `@backend/app/api/routes/banlist_configs.py` around lines 64 - 78, The
update_banlist and delete_banlist routes allow mutating public ban lists because
banlist_crud.get() skips ownership for obj.is_public and
banlist_crud.update/delete do not verify owner; add an explicit ownership check
in both route handlers after fetching obj (in update_banlist and delete_banlist)
that compares the object's owner (obj.organization_id or obj.owner_id) against
the caller's organization (use the AuthDep value currently ignored in the
signature or the organization_id parameter) and raise an authorization error
(HTTP 403) if they differ; ensure the check runs for mutating operations on
public lists so only the owning organization can update or delete, then proceed
to call banlist_crud.update or banlist_crud.delete when the ownership check
passes.
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@backend/app/api/deps.py`:
- Line 70: Add a trailing newline at the end of the file so the file ends with a
newline character; specifically update the file containing the TenantDep
definition (Annotated[Tenant, Depends(get_tenant)]) to ensure there is a newline
after the final line to satisfy W292.
In `@backend/app/tests/test_banlists_integration.py`:
- Line 14: The tests currently build a DEFAULT_QUERY string and append
?organization_id=...&project_id=... to helper request URLs, but the application
now uses TenantDep headers (X-Org-Id / X-Project-Id) so those query params are
dead; remove DEFAULT_QUERY and any usage of the query string in helper functions
and test calls in test_banlists_integration.py (and the helper methods
referenced there) and ensure requests rely solely on the TenantDep headers
already being set (X-Org-Id, X-Project-Id) to derive tenant context.
- Around line 242-251: The test test_delete_wrong_owner currently sends a stale
query string in the DELETE call
(f"{BASE_URL}{ban_id}/?organization_id=999&project_id=999") which is ignored;
remove the query params and call integration_client.delete with the resource
path only (e.g. f"{BASE_URL}{ban_id}/") and continue to pass the owner headers
via self._headers(999, 999) so the request relies on headers for auth/ownership.
🧹 Nitpick comments (1)
backend/app/api/deps.py (1)
48-67: Falsy check onintheaders will reject valid0IDs and is partially redundant.
Header(...)already makes the headers required — FastAPI returns 422 if they're missing or non-integer. Theif not x_org_idcheck on line 58 only additionally rejects0. If org/project IDs are always positive, consider making the intent explicit with<= 0; if0could be valid, this is a bug.Suggested clarification
- if not x_org_id or not x_project_id: + if x_org_id <= 0 or x_project_id <= 0: raise HTTPException( status_code=status.HTTP_400_BAD_REQUEST, detail="Missing tenant headers", )
| sa.Column('organization_id', sa.Integer(), nullable=False), | ||
| sa.Column('project_id', sa.Integer(), nullable=False), |
There was a problem hiding this comment.
similar to this we might need to introduce them to other table as well
| def create_banlist( | ||
| payload: BanListCreate, | ||
| session: SessionDep, | ||
| organization_id: int, | ||
| project_id: int, | ||
| _: AuthDep, | ||
| ): | ||
| response_model = banlist_crud.create(session, payload, organization_id, project_id) | ||
| return APIResponse.success_response(data=response_model) |
| self, | ||
| session: Session, | ||
| id: UUID, | ||
| organization_id: int, |
There was a problem hiding this comment.
you can just pass the schema you made containing both org id and project id, and then use it later this way , schema.organization_id, schema.project_id
There was a problem hiding this comment.
sorry, didn't get you
rkritika1508
force-pushed
the
feat/banlist-management
branch
from
0adff7e to
8c0b7de
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/tests/test_validator_configs_integration.py (1)
59-61:⚠️ Potential issue | 🟠 MajorInconsistent URL construction —
update_validatoris missing the trailing slash.
get_validator(line 47) anddelete_validator(line 65) use{BASE_URL}{validator_id}/{DEFAULT_QUERY_PARAMS}(with trailing/), butupdate_validatoruses{BASE_URL}{validator_id}{DEFAULT_QUERY_PARAMS}(no trailing/). This produces a different path that may trigger a 307 redirect in FastAPI, potentially causing the PATCH body to be dropped.Fix: add trailing slash
def update_validator(self, client, validator_id, payload): """Helper to update a validator.""" - return client.patch(f"{BASE_URL}{validator_id}{DEFAULT_QUERY_PARAMS}", json=payload) + return client.patch(f"{BASE_URL}{validator_id}/{DEFAULT_QUERY_PARAMS}", json=payload)
🤖 Fix all issues with AI agents
In `@backend/app/tests/test_banlists_integration.py`:
- Line 5: The clear_database fixture is importing the production engine (engine)
instead of using the test_engine provided by conftest.py; update clear_database
to use the conftest test_engine (or move the clear_database fixture into
conftest alongside other DB fixtures) so it runs against the test database
instance; locate the clear_database fixture in test_banlists_integration.py (and
similarly in test_validator_configs_integration.py) and replace the engine
reference with test_engine (or relocate the entire fixture into conftest.py) to
ensure the fixture uses the same test_engine configuration as other DB fixtures.
In `@backend/app/tests/test_validator_configs_integration.py`:
- Around line 5-6: The clear_database fixture is importing and using the
production engine from app.core.db instead of the test_engine set up in
conftest; update the import in test_validator_configs_integration.py to use
test_engine (the same symbol used in conftest) and ensure any references inside
the clear_database fixture call/operate on test_engine rather than engine so the
fixture clears the test database during integration tests.
🧹 Nitpick comments (2)
backend/app/tests/conftest.py (1)
86-93: Redundant deletes inseed_db—clean_db(autouse) already clears all tables.Since
clean_dbisautouse=Trueand runs before every test function (clearing all tables), the explicitdelete(BanList)/delete(ValidatorConfig)inseed_dbare redundant. You can simplify to just seed and yield.Simplified seed_db
`@pytest.fixture`(scope="function") def seed_db(): with Session(test_engine) as session: - session.exec(delete(BanList)) - session.exec(delete(ValidatorConfig)) - session.commit() seed_test_data(session) yieldbackend/app/tests/test_validator_configs_integration.py (1)
49-57:list_validatorsrebuildsDEFAULT_QUERY_PARAMSinline instead of reusing the constant.Lines 51-54 duplicate the same string that
DEFAULT_QUERY_PARAMS(line 17-20) already defines.DRY fix
def list_validators(self, client, **query_params): """Helper to list validators with optional filters.""" - params_str = ( - f"?organization_id={VALIDATOR_INTEGRATION_ORGANIZATION_ID}" - f"&project_id={VALIDATOR_INTEGRATION_PROJECT_ID}" - ) + params_str = DEFAULT_QUERY_PARAMS if query_params: params_str += "&" + "&".join(f"{k}={v}" for k, v in query_params.items()) return client.get(f"{BASE_URL}{params_str}")
4 participants
Summary
Target issue is #33.
Explain the motivation for making this change. What existing problem does the pull request solve?
Currently, we have no way to manage ban list for each NGOs. So, we want to build APIs which manage ban list and allow other NGOs to use them as well.
The following APIs will be added -
File-by-File Breakdown
APIs
Checklist
Before submitting a pull request, please ensure that you mark these task.
fastapi run --reload app/main.pyordocker compose upin the repository root and test.Notes
Please add here if any other information is required for the reviewer.
Summary by CodeRabbit
New Features
Database
Tests
Chores